Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IMPROVE][Slack import] Parse channel and user mentions #17637

Merged
merged 7 commits into from
Jun 19, 2020

Conversation

lpilz
Copy link
Contributor

@lpilz lpilz commented May 13, 2020

This is the start of work to make the channel mentions clickable links. It eventually Closes #17593.

@pierre-lehnen-rc pierre-lehnen-rc self-requested a review May 14, 2020 15:11
@lpilz
Copy link
Contributor Author

lpilz commented May 17, 2020

This doesn't fix the issue yet. I am still searching for where in the code mentions (user and room) are being made clickable. @pierre-lehnen-rc do you happen to know that by any chance?

@lpilz lpilz changed the title [FIX] Mentions of channels in message text [FIX] Slack import: Mentions of channels in message text May 17, 2020
@pierre-lehnen-rc
Copy link
Contributor

This doesn't fix the issue yet. I am still searching for where in the code mentions (user and room) are being made clickable. @pierre-lehnen-rc do you happen to know that by any chance?

The messages are parsed by this lib:

https://github.com/RocketChat/Rocket.Chat/blob/develop/app/mentions/lib/MentionsParser.js

@lpilz
Copy link
Contributor Author

lpilz commented May 18, 2020

There are still issues. I think the permissions one might be client side though.

  • Major bug importing the settings obj was resolved (correcly? pls check)
  • User mentions are clickable
    • user names do not get replaced with real names even when UI_Use_Real_Name is true
  • when clicking on channel mentions, a red box with "not allowed" appears
    • console output only has: TypeError: "'name' member of PermissionDescriptor 'microphone' is not a valid value for enumeration PermissionName." which is prob not related
    • server logs indicate it happens when getRoomById is called
  • pattern settings are not properly applied to the MentionsParser obj (clickable link ends at umlauts even thought the whole channel name (incl umlaut) was properly added to mentions array)
    • UTF8_Names_Validation is still [0-9a-zA-Z-_.]+ when UI_Allow_room_names_with_special_chars is set

@lgtm-com
Copy link

lgtm-com bot commented May 18, 2020

This pull request fixes 1 alert when merging a004a99 into 29eef3d - view on LGTM.com

fixed alerts:

  • 1 for Property access on null or undefined

@pierre-lehnen-rc
Copy link
Contributor

Any errors not related to the import process itself can be left out of the PR and registered as an issue, but do feel free to open a new PR with any fixes you'd like to include.

@lpilz lpilz mentioned this pull request May 24, 2020
12 tasks
@lpilz
Copy link
Contributor Author

lpilz commented May 24, 2020

I separated the fix out into #17776

Also, changing UI_Allow_room_names_with_special_chars doesn't affect UTF8_Names_Validation. @pierre-lehnen-rc do you know where one can implement such a trigger or where one would look for one? If this is fixed, the Umlaut-issue should work, as in app/importer/server/classes/ImporterBase.js, the special chars setting is enabled for the import.

I found the issue. It is that the default UTF8 encoding regex does not contain any non-ASCII chars

@lpilz
Copy link
Contributor Author

lpilz commented May 24, 2020

A new issue, importing the general channel from slack, has cropped up. This is not handled correctly yet. The problem is, that the general channel in the import slack for me is called "allgmein" it gets imported as <a class="mention-link mention-link--room" data-channel="allgemein">#allgemein</a>. This is prob due to using the name strings and not the names attached to the room objects. Will investigate further.

@lgtm-com
Copy link

lgtm-com bot commented May 24, 2020

This pull request fixes 1 alert when merging 978f9ea into 0543346 - view on LGTM.com

fixed alerts:

  • 1 for Property access on null or undefined

@lgtm-com
Copy link

lgtm-com bot commented May 28, 2020

This pull request fixes 1 alert when merging 4e0055b into 0d2723f - view on LGTM.com

fixed alerts:

  • 1 for Property access on null or undefined

@lgtm-com
Copy link

lgtm-com bot commented May 28, 2020

This pull request fixes 1 alert when merging adc3a0d into 0d2723f - view on LGTM.com

fixed alerts:

  • 1 for Property access on null or undefined

@lpilz lpilz force-pushed the fix/slack-import-mentions branch 2 times, most recently from 903f2c3 to ca13980 Compare May 28, 2020 22:03
@lpilz lpilz mentioned this pull request May 28, 2020
10 tasks
@lpilz lpilz marked this pull request as ready for review May 28, 2020 22:12
@lgtm-com
Copy link

lgtm-com bot commented May 28, 2020

This pull request fixes 1 alert when merging ca13980 into 0d2723f - view on LGTM.com

fixed alerts:

  • 1 for Property access on null or undefined

@lpilz
Copy link
Contributor Author

lpilz commented May 28, 2020

I unfortunately can't lint the commit, as meteor npm run lint returns

TypeError: Cannot read property 'range' of null
npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! Failed at the Rocket.Chat@3.3.0-develop jslint script.

@lgtm-com
Copy link

lgtm-com bot commented May 29, 2020

This pull request fixes 1 alert when merging 305e58e into cae6aeb - view on LGTM.com

fixed alerts:

  • 1 for Property access on null or undefined

@lgtm-com
Copy link

lgtm-com bot commented May 29, 2020

This pull request fixes 1 alert when merging 8c1a38e into cae6aeb - view on LGTM.com

fixed alerts:

  • 1 for Property access on null or undefined

@lpilz
Copy link
Contributor Author

lpilz commented May 29, 2020

As far as I see it, work is complete. Depends on #17776.

@lpilz lpilz changed the title [FIX] Slack import: Mentions of channels in message text [FIX] Slack import: Made channel and user mentions clickable May 29, 2020
@engelgabriel engelgabriel added this to the 3.4.0 milestone Jun 2, 2020
@TemaSM
Copy link

TemaSM commented Jun 8, 2020

Another Slack's importer issue: #17855

@lpilz lpilz force-pushed the fix/slack-import-mentions branch from 8c1a38e to e42022b Compare June 9, 2020 13:35
@lpilz
Copy link
Contributor Author

lpilz commented Jun 9, 2020

Rebased branch after merge of #17776. Ready to merge now.

@rodrigok rodrigok self-assigned this Jun 19, 2020
@pierre-lehnen-rc pierre-lehnen-rc self-assigned this Jun 19, 2020
@rodrigok rodrigok removed their assignment Jun 19, 2020
@sampaiodiego sampaiodiego changed the title [FIX] Slack import: Made channel and user mentions clickable [FIX] Slack import: Parse channel and user mentions Jun 19, 2020
@sampaiodiego sampaiodiego changed the title [FIX] Slack import: Parse channel and user mentions [IMPROVE] Slack import: Parse channel and user mentions Jun 19, 2020
@sampaiodiego sampaiodiego merged commit 117e623 into RocketChat:develop Jun 19, 2020
@sampaiodiego sampaiodiego mentioned this pull request Jun 30, 2020
@rodrigok rodrigok changed the title [IMPROVE] Slack import: Parse channel and user mentions [IMPROVE][Slack import] Parse channel and user mentions Jul 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Slack Importer doesn't correctly import channel mentions in message text
6 participants